-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use English for string comparisons #14910
Conversation
82f1e2c
to
a516325
Compare
028364d
to
1b580d3
Compare
1b580d3
to
e0b9d39
Compare
awx/main/tasks/system.py
Outdated
# xxx.rowcount on update/delete shoud be used to determined affected rows. | ||
# Not sure how an expcetion is thrown from update_computed_fields() | ||
else: | ||
logger.debug('Exiting duplicate update_inventory_computed_fields task. {}'.format(str(e))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've lost the raise
in this exception handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added raise on 805
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, the "did not affect any rows" message is coming from Django instead of the db in this case: https://github.com/django/django/blob/eff21d8e7a1cb297aedf1c702668b590a1b618f3/django/db/models/base.py#L1107
It's that one in particular, since i.update_computed_fields()
makes use of .save(update_fields=...)
.
You are correct that in this case the exception won't get a sqlstate
, so this flow of logic looks ok to me.
e0b9d39
to
901aad7
Compare
901aad7
to
fc63655
Compare
awx/main/utils/common.py
Outdated
if cause and hasattr(cause, 'sqlstate'): | ||
sqlstate = cause.sqlstate | ||
sqlstate_str = psycopg.errors.lookup(sqlstate) | ||
logger.error('SQL Error state: {} - {}'.format(sqlstate, sqlstate_str)) | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I would expect that this exception block would always get a sqlstate
, if we are coding defensively the raise
should be outside of the if
block, just in case that isn't so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, my only recommendation now is to probably code more defensively around the raise
in utils/common.py.
8733bb1
to
7000b6a
Compare
c0b2ddd
to
6ee96d8
Compare
6ee96d8
to
2fff717
Compare
f019fc4
to
a1e9a71
Compare
awx/main/tasks/system.py
Outdated
@@ -786,11 +795,17 @@ def update_inventory_computed_fields(inventory_id): | |||
return | |||
i = i[0] | |||
try: | |||
i.update_computed_fields() | |||
if i.update_computed_fields() == 0: | |||
logger.debug('Duplicate update_inventory_computed_fields task.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per offline conversation, I do not believe that this logging will fire off.
Inventory.update_computed_fields
does not seem to return a value (unless I missed it)- edit: I've now commented on your changes that attempted to add that
- Django's internals raise the "did not affect any rows" error, not any database backend (ref: https://github.com/django/django/blob/eff21d8e7a1cb297aedf1c702668b590a1b618f3/django/db/models/base.py#L1107). Therefore we'll get a
DatabaseError
anyway, bypassing this line, and that exception will not have asqlstate
attached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New comment.
awx/main/models/inventory.py
Outdated
logger.debug("Finished updating inventory computed fields, pk={0}, in {1:.3f} seconds".format(self.pk, time.time() - start_time)) | ||
return rowcount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I oppose this change for the following reasons:
- making changes here has crept out of the scope of this PR
- the
bulk_update
is not the only save occurring in this method, so returning only its row count isn't very idiomatic - this does absolutely nothing to address the fact that
iobj.save(update_fields=computed_fields.keys())
is going to raise theDatabaseError("Save with update_fields did not affect any rows.")
exception in the case we care about
eeda300
to
fbeb0de
Compare
fbeb0de
to
7b8278e
Compare
possible regression because of this change #15016 |
SUMMARY
This work is to remove string validation using comparisons of English literals for comparison.
This replaces validation with error/op codes as a universal approach to validation and comparison
ISSUE TYPE
COMPONENT NAME
AWX VERSION